Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for AD7091R-2, AD7091R-4 and AD7091R-8 #595

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SaikiranGudla
Copy link
Contributor

@SaikiranGudla SaikiranGudla commented Sep 2, 2024

Add ad7091r.py driver
Add test script, example code
Update index.rst, init.py, supported_parts.md files Add xml emu context file
Update hardware_map.yml file for test emulation

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • Hardware:
  • OS:

Documentation

If this is a new feature or example please mention or link any documentation. All new hardware interface classes require documentation.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have signed off all commits and they contain "Signed-off by: "
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@SaikiranGudla SaikiranGudla force-pushed the ad7091r_support branch 4 times, most recently from 3e595a5 to c22d8ea Compare September 3, 2024 05:25
Copy link
Collaborator

@tfcollins tfcollins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@machschmitt machschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SaikiranGudla,

Looks good overall. This is a bit similar to some old drafts I had for ad7091r8 (https://github.com/machschmitt/pyadi-iio/tree/staging/ad7091r8), but this seems to be leveraging more of pyadi abstraction so probably better to improve this PR's branch.
I'm getting my rpi setup to test this with ad7091r8 and may come back with more comments soon.
Meanwhile, some questions: do you have an eval board to test ad7091r8? Are you still working on ad7091r8 pyadi support?
If you don't have an eval, I'll help test updates to ad7091r8 pyadi support as you update this PR.
If you have switched to other projects, then let me know so I can implement the remaining changes to get this merged.

adi/ad7091r.py Outdated
Comment on lines 151 to 153
def to_volts(self, index, val):
"""Converts raw value to SI"""
_scale = self.channel[index].scale
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the drivers are correct, this should return a value in milli volts (mV).
I would suggest to update the comment and function name to mitigate confusion.

adi/ad7091r.py Outdated
Comment on lines 98 to 104
def offset(self):
"""AD7091r channel offset value"""
return self._get_iio_attr_str(self.name, "offset", False)

@offset.setter
def offset(self, value):
self._set_iio_attr(self.name, "offset", False, str(Decimal(value).real))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ADCs have single-ended unipolar channels that don't really need an _offset attribute and the drivers do not expose any offset. The pyadi support should probably not provide any offset either.

adi/ad7091r.py Outdated
Comment on lines 111 to 113
@scale.setter
def scale(self, value):
self._set_iio_attr(self.name, "scale", False, str(Decimal(value).real))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For AD7091R-2/-4/-8, the _scale is a read-only attribute so it is better to remove the scale setter.

@@ -0,0 +1 @@
<?xml version="1.0" encoding="utf-8"?><!DOCTYPE context [<!ELEMENT context (device | context-attribute)*><!ELEMENT context-attribute EMPTY><!ELEMENT device (channel | attribute | debug-attribute | buffer-attribute)*><!ELEMENT channel (scan-element?, attribute*)><!ELEMENT attribute EMPTY><!ELEMENT scan-element EMPTY><!ELEMENT debug-attribute EMPTY><!ELEMENT buffer-attribute EMPTY><!ATTLIST context name CDATA #REQUIRED description CDATA #IMPLIED><!ATTLIST context-attribute name CDATA #REQUIRED value CDATA #REQUIRED><!ATTLIST device id CDATA #REQUIRED name CDATA #IMPLIED><!ATTLIST channel id CDATA #REQUIRED type (input|output) #REQUIRED name CDATA #IMPLIED><!ATTLIST scan-element index CDATA #REQUIRED format CDATA #REQUIRED scale CDATA #IMPLIED><!ATTLIST attribute name CDATA #REQUIRED filename CDATA #IMPLIED value CDATA #IMPLIED><!ATTLIST debug-attribute name CDATA #REQUIRED value CDATA #IMPLIED><!ATTLIST buffer-attribute name CDATA #REQUIRED value CDATA #IMPLIED>]><context name="serial" description="no-OS 0.1" ><context-attribute name="hw_carrier" value="SDP_K1" /><context-attribute name="hw_mezzanine" value="EVAL-AD7091R-8ARDZ" /><context-attribute name="hw_name" value="EVAL-AD7091R-8ARDZ" /><context-attribute name="uri" value="serial:/dev/ttyS0,230400,8n1n" /><context-attribute name="serial,port" value="/dev/ttyS0" /><context-attribute name="serial,description" value="ttyS0" /><device id="iio:device0" name="ad7091r-8" ><channel id="voltage0" name="Chn0" type="input" ><scan-element index="0" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage0_raw" value="0" /><attribute name="scale" filename="in_voltage0_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage0_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage0_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage0_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage0_thresh_either_hysteresis" value="511" /><channel id="voltage1" name="Chn1" type="input" ><scan-element index="1" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage1_raw" value="0" /><attribute name="scale" filename="in_voltage1_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage1_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage1_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage1_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage1_thresh_either_hysteresis" value="511" /><channel id="voltage2" name="Chn2" type="input" ><scan-element index="2" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage2_raw" value="0" /><attribute name="scale" filename="in_voltage2_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage2_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage2_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage2_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage2_thresh_either_hysteresis" value="511" /><channel id="voltage3" name="Chn3" type="input" ><scan-element index="3" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage3_raw" value="0" /><attribute name="scale" filename="in_voltage3_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage3_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage3_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage3_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage3_thresh_either_hysteresis" value="511" /><channel id="voltage4" name="Chn4" type="input" ><scan-element index="4" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage4_raw" value="0" /><attribute name="scale" filename="in_voltage4_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage4_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage4_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage4_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage4_thresh_either_hysteresis" value="511" /><channel id="voltage5" name="Chn5" type="input" ><scan-element index="5" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage5_raw" value="0" /><attribute name="scale" filename="in_voltage5_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage5_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage5_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage5_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage5_thresh_either_hysteresis" value="511" /><channel id="voltage6" name="Chn6" type="input" ><scan-element index="6" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage6_raw" value="0" /><attribute name="scale" filename="in_voltage6_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage6_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage6_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage6_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage6_thresh_either_hysteresis" value="511" /><channel id="voltage7" name="Chn7" type="input" ><scan-element index="7" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage7_raw" value="0" /><attribute name="scale" filename="in_voltage7_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage7_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage7_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage7_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage7_thresh_either_hysteresis" value="511" /><debug-attribute name="direct_reg_access" value="0" /></device><device id="trigger0" name="ad7091r_iio_trigger" ></device></context>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you get this xml from?
Neither Linux nor No-OS ad7091r8 drivers provide _offset attributes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the device xml retrieved from AD7091R-8 connected on a raspberrypi:

<?xml version="1.0" encoding="utf-8"?><!DOCTYPE context [<!ELEMENT context (device | context-attribute)*><!ELEMENT context-attribute EMPTY><!ELEMENT device (channel | attribute | debug-attribute | buffer-attribute)*><!ELEMENT channel (scan-element?, attribute*)><!ELEMENT attribute EMPTY><!ELEMENT scan-element EMPTY><!ELEMENT debug-attribute EMPTY><!ELEMENT buffer-attribute EMPTY><!ATTLIST context name CDATA #REQUIRED description CDATA #IMPLIED><!ATTLIST context-attribute name CDATA #REQUIRED value CDATA #REQUIRED><!ATTLIST device id CDATA #REQUIRED name CDATA #IMPLIED><!ATTLIST channel id CDATA #REQUIRED type (input|output) #REQUIRED name CDATA #IMPLIED><!ATTLIST scan-element index CDATA #REQUIRED format CDATA #REQUIRED scale CDATA #IMPLIED><!ATTLIST attribute name CDATA #REQUIRED filename CDATA #IMPLIED value CDATA #IMPLIED><!ATTLIST debug-attribute name CDATA #REQUIRED value CDATA #IMPLIED><!ATTLIST buffer-attribute name CDATA #REQUIRED value CDATA #IMPLIED>]><context name="network" description="192.168.1.3 Linux raspberrypi 6.8.0-rc4-v8+ #4 SMP PREEMPT Thu Feb 15 10:45:25 -03 2024 aarch64" ><context-attribute name="local,kernel" value="6.8.0-rc4-v8+" /><context-attribute name="uri" value="ip:192.168.1.3" /><context-attribute name="ip,ip-addr" value="192.168.1.3" /><device id="iio:device0" name="ad7091r-8" ><channel id="voltage1" type="input" ><attribute name="raw" filename="in_voltage1_raw" value="2603" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage5" type="input" ><attribute name="raw" filename="in_voltage5_raw" value="0" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage2" type="input" ><attribute name="raw" filename="in_voltage2_raw" value="533" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage6" type="input" ><attribute name="raw" filename="in_voltage6_raw" value="1076" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage3" type="input" ><attribute name="raw" filename="in_voltage3_raw" value="1922" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage7" type="input" ><attribute name="raw" filename="in_voltage7_raw" value="1747" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage0" type="input" ><attribute name="raw" filename="in_voltage0_raw" value="2229" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage4" type="input" ><attribute name="raw" filename="in_voltage4_raw" value="2376" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><attribute name="waiting_for_supplier" value="0" /></device></context>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from the tinyiiod fw I've created. I'll update this with the one that you've motioned here.

Comment on lines 37 to 38
# Set up AD7091r8
ad7091r8_dev = ad7091rx(uri="serial:COM46,230400,8n1", device_name="ad7091r-8")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use argparse to make the example initialization more flexible and add parameters for specifying the URL and which ADC (r-2/r-4/r-8) is available.

@pytest.mark.parametrize("classname", [(classname)])
@pytest.mark.parametrize("channel", [0])
def test_ad7091rx_rx_data(test_dma_rx, iio_uri, classname, channel):
test_dma_rx(iio_uri, classname, channel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the appropriate way to test current ad7091r-2/-4/-8 support.
I've heard that raspberry pies can optimize repeated SPI transfers so that their data go through the DMA path. Though, the Linux driver doesn't currently support buffered readings.
No-OS ad7091r8 driver does support buffered readings, but I'm not aware of any HDL project that enables transfers going through DMA. So, I think this test will not succeed in practice, unless test_dma_rx() can fall back to non-DMA transfers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python -m pytest -vs -k ad7091rx --emu throws Exception: Buffer all zeros. Maybe better to not assume the device is buffer capable in tests. Maybe check for buffer support prior to test_dma_rx()? Or maybe catch the exception?

Copy link
Contributor

@machschmitt machschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this ad7091r-2/-4/-8 support with AD7091R-8 connected to a raspberrypi.
Basic functionality (raw data read) works!
Other features (threshold/events, buffer) are either not fully supported by the drivers or by libiio so I would drop those for now.

ad7091r8_dev.rx_enabled_channels = [chn]
ad7091r8_dev.rx_buffer_size = 100

data = ad7091r8_dev.rx()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running the example with AD7091R-8 on a rpi, this line throws OSError: [Errno 22] Invalid argument after some null check in _create_buffer(), probably due to Linux not supporting buffered reads for ad7091r-2/-4/-8 devices. Would be nice to check if the device is buffer capable then run the capture or print a message depending on buffer support.

print(f"Raw value read from channel0 is {raw}")

# Set threshold falling and rising values for channel 0
ad7091r8_dev.channel[0].thresh_rising_value = 0x10F
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running the example with AD7091R-8 on a rpi, this line throws KeyError: 'thresh_rising_value'.
The thresholds are supported as IIO events but it looks like libiio doesn't support IIO events currently. I would drop the attempt to support events/thresholds and maybe leave a comment mentioning they can be implemented when libiio gets to support them.
I do get raw data if I only run the first part of the example.

@@ -0,0 +1 @@
<?xml version="1.0" encoding="utf-8"?><!DOCTYPE context [<!ELEMENT context (device | context-attribute)*><!ELEMENT context-attribute EMPTY><!ELEMENT device (channel | attribute | debug-attribute | buffer-attribute)*><!ELEMENT channel (scan-element?, attribute*)><!ELEMENT attribute EMPTY><!ELEMENT scan-element EMPTY><!ELEMENT debug-attribute EMPTY><!ELEMENT buffer-attribute EMPTY><!ATTLIST context name CDATA #REQUIRED description CDATA #IMPLIED><!ATTLIST context-attribute name CDATA #REQUIRED value CDATA #REQUIRED><!ATTLIST device id CDATA #REQUIRED name CDATA #IMPLIED><!ATTLIST channel id CDATA #REQUIRED type (input|output) #REQUIRED name CDATA #IMPLIED><!ATTLIST scan-element index CDATA #REQUIRED format CDATA #REQUIRED scale CDATA #IMPLIED><!ATTLIST attribute name CDATA #REQUIRED filename CDATA #IMPLIED value CDATA #IMPLIED><!ATTLIST debug-attribute name CDATA #REQUIRED value CDATA #IMPLIED><!ATTLIST buffer-attribute name CDATA #REQUIRED value CDATA #IMPLIED>]><context name="serial" description="no-OS 0.1" ><context-attribute name="hw_carrier" value="SDP_K1" /><context-attribute name="hw_mezzanine" value="EVAL-AD7091R-8ARDZ" /><context-attribute name="hw_name" value="EVAL-AD7091R-8ARDZ" /><context-attribute name="uri" value="serial:/dev/ttyS0,230400,8n1n" /><context-attribute name="serial,port" value="/dev/ttyS0" /><context-attribute name="serial,description" value="ttyS0" /><device id="iio:device0" name="ad7091r-8" ><channel id="voltage0" name="Chn0" type="input" ><scan-element index="0" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage0_raw" value="0" /><attribute name="scale" filename="in_voltage0_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage0_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage0_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage0_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage0_thresh_either_hysteresis" value="511" /><channel id="voltage1" name="Chn1" type="input" ><scan-element index="1" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage1_raw" value="0" /><attribute name="scale" filename="in_voltage1_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage1_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage1_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage1_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage1_thresh_either_hysteresis" value="511" /><channel id="voltage2" name="Chn2" type="input" ><scan-element index="2" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage2_raw" value="0" /><attribute name="scale" filename="in_voltage2_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage2_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage2_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage2_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage2_thresh_either_hysteresis" value="511" /><channel id="voltage3" name="Chn3" type="input" ><scan-element index="3" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage3_raw" value="0" /><attribute name="scale" filename="in_voltage3_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage3_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage3_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage3_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage3_thresh_either_hysteresis" value="511" /><channel id="voltage4" name="Chn4" type="input" ><scan-element index="4" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage4_raw" value="0" /><attribute name="scale" filename="in_voltage4_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage4_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage4_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage4_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage4_thresh_either_hysteresis" value="511" /><channel id="voltage5" name="Chn5" type="input" ><scan-element index="5" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage5_raw" value="0" /><attribute name="scale" filename="in_voltage5_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage5_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage5_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage5_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage5_thresh_either_hysteresis" value="511" /><channel id="voltage6" name="Chn6" type="input" ><scan-element index="6" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage6_raw" value="0" /><attribute name="scale" filename="in_voltage6_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage6_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage6_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage6_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage6_thresh_either_hysteresis" value="511" /><channel id="voltage7" name="Chn7" type="input" ><scan-element index="7" format="be:u12/16>>0" /><attribute name="raw" filename="in_voltage7_raw" value="0" /><attribute name="scale" filename="in_voltage7_scale" value=" 0.6105006337" /><attribute name="offset" filename="in_voltage7_offset" value="0" /><attribute name="thresh_falling_value" filename="in_voltage7_thresh_falling_value" value="0" /></channel><attribute name="thresh_rising_value" filename="in_voltage7_thresh_rising_value" value="4095" /><attribute name="thresh_either_hysteresis" filename="in_voltage7_thresh_either_hysteresis" value="511" /><debug-attribute name="direct_reg_access" value="0" /></device><device id="trigger0" name="ad7091r_iio_trigger" ></device></context>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the device xml retrieved from AD7091R-8 connected on a raspberrypi:

<?xml version="1.0" encoding="utf-8"?><!DOCTYPE context [<!ELEMENT context (device | context-attribute)*><!ELEMENT context-attribute EMPTY><!ELEMENT device (channel | attribute | debug-attribute | buffer-attribute)*><!ELEMENT channel (scan-element?, attribute*)><!ELEMENT attribute EMPTY><!ELEMENT scan-element EMPTY><!ELEMENT debug-attribute EMPTY><!ELEMENT buffer-attribute EMPTY><!ATTLIST context name CDATA #REQUIRED description CDATA #IMPLIED><!ATTLIST context-attribute name CDATA #REQUIRED value CDATA #REQUIRED><!ATTLIST device id CDATA #REQUIRED name CDATA #IMPLIED><!ATTLIST channel id CDATA #REQUIRED type (input|output) #REQUIRED name CDATA #IMPLIED><!ATTLIST scan-element index CDATA #REQUIRED format CDATA #REQUIRED scale CDATA #IMPLIED><!ATTLIST attribute name CDATA #REQUIRED filename CDATA #IMPLIED value CDATA #IMPLIED><!ATTLIST debug-attribute name CDATA #REQUIRED value CDATA #IMPLIED><!ATTLIST buffer-attribute name CDATA #REQUIRED value CDATA #IMPLIED>]><context name="network" description="192.168.1.3 Linux raspberrypi 6.8.0-rc4-v8+ #4 SMP PREEMPT Thu Feb 15 10:45:25 -03 2024 aarch64" ><context-attribute name="local,kernel" value="6.8.0-rc4-v8+" /><context-attribute name="uri" value="ip:192.168.1.3" /><context-attribute name="ip,ip-addr" value="192.168.1.3" /><device id="iio:device0" name="ad7091r-8" ><channel id="voltage1" type="input" ><attribute name="raw" filename="in_voltage1_raw" value="2603" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage5" type="input" ><attribute name="raw" filename="in_voltage5_raw" value="0" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage2" type="input" ><attribute name="raw" filename="in_voltage2_raw" value="533" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage6" type="input" ><attribute name="raw" filename="in_voltage6_raw" value="1076" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage3" type="input" ><attribute name="raw" filename="in_voltage3_raw" value="1922" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage7" type="input" ><attribute name="raw" filename="in_voltage7_raw" value="1747" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage0" type="input" ><attribute name="raw" filename="in_voltage0_raw" value="2229" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><channel id="voltage4" type="input" ><attribute name="raw" filename="in_voltage4_raw" value="2376" /><attribute name="scale" filename="in_voltage_scale" value="0.610351562" /></channel><attribute name="waiting_for_supplier" value="0" /></device></context>

@pytest.mark.parametrize("classname", [(classname)])
@pytest.mark.parametrize("channel", [0])
def test_ad7091rx_rx_data(test_dma_rx, iio_uri, classname, channel):
test_dma_rx(iio_uri, classname, channel)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

python -m pytest -vs -k ad7091rx --emu throws Exception: Buffer all zeros. Maybe better to not assume the device is buffer capable in tests. Maybe check for buffer support prior to test_dma_rx()? Or maybe catch the exception?

@SaikiranGudla SaikiranGudla force-pushed the ad7091r_support branch 6 times, most recently from e55ec44 to 4245316 Compare December 18, 2024 10:30
Copy link
Contributor

@machschmitt machschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SaikiranGudla, looks good.
Two things only:

  1. Drop IIO event stuff (see comments)
  2. Squash the commits

With those,
Reviewed-by: Marcelo Schmitt [email protected]
Tested-by: Marcelo Schmitt [email protected]

adi/ad7091rx.py Outdated
Comment on lines 103 to 137
@property
def thresh_falling_value(self):
"""Get channel threshold falling value"""
return self._get_iio_attr(self.name, "thresh_falling_value", False)

@thresh_falling_value.setter
def thresh_falling_value(self, value):
"""Set channel threshold falling value"""
self._set_iio_attr(
self.name, "thresh_falling_value", False, str(Decimal(value))
)

@property
def thresh_rising_value(self):
"""Get channel threshold rising value"""
return self._get_iio_attr(self.name, "thresh_rising_value", False)

@thresh_rising_value.setter
def thresh_rising_value(self, value):
"""Set channel threshold rising value"""
self._set_iio_attr(
self.name, "thresh_rising_value", False, str(Decimal(value))
)

@property
def thresh_either_hysteresis(self):
"""Get channel threshold either hysteresis value"""
return self._get_iio_attr(self.name, "thresh_either_hysteresis", False)

@thresh_either_hysteresis.setter
def thresh_either_hysteresis(self, value):
"""Set channel threshold either hysteresis value"""
self._set_iio_attr(
self.name, "thresh_either_hysteresis", False, str(Decimal(value))
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @SaikiranGudla, it will be great to have threshold support at some point but libiio doesn't look like it's already supported. I see in libiio code there is enum iio_event_type in include/iio/iio.h and I even installed libiio v1 from main branch but got KeyError: 'thresh_rising_value' at ad7091r_dev.channel[0].thresh_rising_value = 0x10F.
My suggestion is to drop threshold support for now.

Comment on lines 67 to 76
# Set threshold falling and rising values for channel 0
ad7091r_dev.channel[0].thresh_rising_value = 0x10F
print(
f"Channel 0 threshold rising value set to {ad7091r_dev.channel[0].thresh_rising_value}"
)

ad7091r_dev.channel[0].thresh_falling_value = 0x0F
print(
f"Channel 0 threshold falling value set to {ad7091r_dev.channel[0].thresh_falling_value}"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this since we don't have support for IIO events from libiio.

Comment on lines +78 to +76
# Capture a buffer of 100 samples from channel 0 and display them
chn = 0
ad7091r_dev._rx_data_type = np.int32
ad7091r_dev.rx_output_type = "raw"
ad7091r_dev.rx_enabled_channels = [chn]
ad7091r_dev.rx_buffer_size = 100

data = ad7091r_dev.rx()

print(data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. It works with no-OS support but not with Linux because the driver developer (me) was silly enough to implement buffer support in no-OS but not in Linux 😅 . It would be nice if we somehow find a way to check whether the driver supports buffered reading and only do the buffer thing if it does. That way the example will not fail with linux. Though I think it is also fine to have it like it is now. Maybe I take the time to implement buffered read in linux someday.

@SaikiranGudla SaikiranGudla force-pushed the ad7091r_support branch 2 times, most recently from 0b5b610 to 3f32f61 Compare December 19, 2024 16:14
@SaikiranGudla
Copy link
Contributor Author

Resolved all the review comments and squashed the commits

Copy link
Contributor

@machschmitt machschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, thank you for adding AD7091R-2/-4/-8 pyadi support.

@SaikiranGudla
Copy link
Contributor Author

Please add more testing to the component channels. Here is an example https://github.com/analogdevicesinc/pyadi-iio/blob/main/test/test_ad3552r.py Doc: https://analogdevicesinc.github.io/pyadi-iio/dev/test_attr.html#test.attr_tests.attribute_single_value

Please fix CI issues as well

Resolved this! May I know if the PR is good to be merged now, @tfcollins?

@tfcollins
Copy link
Collaborator

@SaikiranGudla please review CI issues. All of your current open PRs will be ignored unless CI is passing first

Add ad7091r.py driver
Add test script, example code
Update index.rst, init.py, supported_parts.md files
Add xml emu context file
Update hardware_map.yml file for test emulation

Signed-off-by: SGudla <[email protected]>
@SaikiranGudla
Copy link
Contributor Author

@SaikiranGudla please review CI issues. All of your current open PRs will be ignored unless CI is passing first

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants